👋 Welcome back hgreule! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

@SirYwell The following label will be automatically applied to this pull request:

• hotspot-compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

Webrevs

• 01: Full - Incremental (6a8d842f)
• 00: Full (6a392224)

Thank you for working on this, @SirYwell. This seems like a tricky problem. To be honest, the fix seems a bit hacky. Have you explored any alternatives to this method of delaying the optimizations?

I kicked off some testing in the meantime that I will report back upon completion.

Thanks for running tests. I tried delaying until post loop opts, but that prevents some vectorization and isn't really less hacky I guess. I didn't find any other good existing approach. Calculating Value before Ideal would work, but I assume that it is rarely useful, with Div/Mod being an exception. I a dream world, I guess we would have e-graphs or something similar, which would allow calculating a more precise type from different alternatives.

If you can think of a better approach, please let me know.

Thinking about it a bit more, I think your fix is too superficial. If the discovery of the constant is slightly delayed, nothing is folded again. Consider the followig program for an example:

class Test {
    static boolean test(int x, boolean flag) {
        Integer a;
        if (flag) {
            a = 171384;
        } else {
            a = 2902;
        }

        return x % a >= a;
    }

    public static void main(String[] args) {
        for (int i = 0; i < 20000; i++) {
            if (test(i, false)) {
                throw new RuntimeException("wrong result");
            }
        }
    }
}

In my opinion, the benefits do not outweigh the drawbacks for this PR. A better solution would probably be to delay the expansion of the Mod and Div nodes to post-loop optimizations and extend Superword to expand Div/Mod nodes to shifts. However, this is quite a bit of complexity, which raises if this complexity is worth it (@eme64 probably has opinions and/or guidance on this).

Thinking about it a bit more, I think your fix is too superficial. If the discovery of the constant is slightly delayed, nothing is folded again. Consider the followig program for an example:

class Test {
    static boolean test(int x, boolean flag) {
        Integer a;
        if (flag) {
            a = 171384;
        } else {
            a = 2902;
        }

        return x % a >= a;
    }

    public static void main(String[] args) {
        for (int i = 0; i < 20000; i++) {
            if (test(i, false)) {
                throw new RuntimeException("wrong result");
            }
        }
    }
}

In my opinion, the benefits do not outweigh the drawbacks for this PR. A better solution would probably be to delay the expansion of the Mod and Div nodes to post-loop optimizations and extend Superword to expand Div/Mod nodes to shifts. However, this is quite a bit of complexity, which raises if this complexity is worth it (@eme64 probably has opinions and/or guidance on this).

I'm not sure about the drawbacks here, but I think optimizing this on the superword level doesn't make things less complicated.

If cases where we end up idealizing before calling Value are a more general problem, I'd say it's worth to also address it on exactly that level: make sure that Value is called before Ideal. I'm just hesitant because I'm not aware of any other situations where this matters. One middle ground here would be some kind of Node::InitialValue(...) (or a better name :) ) that just calls bottom_type() by default and can be overridden for nodes like Mod and Div. Joining that value with the Value calculated later would solve this problem on a different level, but more effectively. But it would also be a more invasive change overall. What it doesn't resolve is users of the node to see it before it gets replaced, which probably isn't that important right now, but can be hindering when e.g., trying to detect floorMod-like patterns.

Thanks for the summary @eme64. I totally agree that it's a bit hacky, but the current state is the least invasive. I'd also be interested in going further steps in the same direction, but I feel like the work increases significantly more than the benefits (at least as long as we don't generalize it to also optimize for loop invariant non-constants, but that's also a lot of work).

@mhaessig do you have test results already?

Manuel and I discussed in the office a little more :)

Can you show us a concrete example, where Div gets Idealized early, and then the generated nodes do not propagate the value range sufficiently precise for the comparison to constant fold?

I suspect that it is the value range "truncation" on the lower bits that are lost in MulHiLNode, but it would be nice to see that example ;)

Because if there is a solution that just improves the Value of the mul/shift/... nodes, that would probably be preferable.

But if we in the end need to build a Value optimization that pattern matches again through the nodes that transform_int_divide generated, that would probably be less nice, given the complexity. And then we should do the delay.

Manuel and I discussed in the office a little more :)

Can you show us a concrete example, where Div gets Idealized early, and then the generated nodes do not propagate the value range sufficiently precise for the comparison to constant fold?

I suspect that it is the value range "truncation" on the lower bits that are lost in MulHiLNode, but it would be nice to see that example ;)

Because if there is a solution that just improves the Value of the mul/shift/... nodes, that would probably be preferable.

But if we in the end need to build a Value optimization that pattern matches again through the nodes that transform_int_divide generated, that would probably be less nice, given the complexity. And then we should do the delay.

One very straightforward example would be something like

static boolean divFold(int a) {
	return a / 100_000 >= 21475;
}

which isn't folded to false with early idealization but works with the changes from this PR and #26143 both applied.

From my analysis, this comes from the the rounding adjustments: We need to round towards zero, so we need to add 1 (=subtract -1) for negative values. We achieve that by an right shift to produce either a 0 or a -1 and then do the subtraction with that value.

The subtraction isn't aware of the relation between the param being negative and the adjustment, and as you said, to recognize that relation, you'd more or less need to recognize that these operations form a division.

Now, I think this is the only case, and it's only off by 1 (and if the sign of the dividend is known, it also isn't a problem), so I'm wondering if there are any common patterns where this would be relevant, otherwise it might really make sense to just delay Mod and accept this edge case for Div.

Thanks very much for the explanation and the nice graph 😊

That helps a lot. It also means that even for cases like @mhaessig showed above:
#27886 (comment)

We could still constant fold the comparison.... as long as the comparison is "relaxed enough". It might be worth having a handfull of examples like that: some that still constant fold, and some that don't because the comparison is too "sharp", and the "rounding error" too large. What do you think?

We could still constant fold the comparison.... as long as the comparison is "relaxed enough". It might be worth having a handfull of examples like that: some that still constant fold, and some that don't because the comparison is too "sharp", and the "rounding error" too large. What do you think?

Do you mean as part of the comment? That should be doable and provide useful context, yes.

Edit: Mod is still harder to constant fold (or even get any precise information from) as we have an additional multiplication and subtraction there. So the example would probably still fail.

Do you mean as part of the comment? That should be doable and provide useful context, yes.

Exactly, yes please 😊

It seems the root cause is a divergence in functionality in GVN between different representations of Div/Mod nodes.

How hard would it be to align the implementation and improve GVN handling for other representations, so the same type is constructed irrespective of the IR shape?

Alternatively, as part of the expansion, new representation can be wrapped in CastII/CastLL with the narrower type of original Div/Mod node.

@iwanowww I'm not quite following your suggestions / questions.

It seems the root cause is a divergence in functionality in GVN between different representations of Div/Mod nodes.

How hard would it be to align the implementation and improve GVN handling for other representations, so the same type is constructed irrespective of the IR shape?

Do you consider the "expanded" versions of Div/Mod as a "different representation of Div/Mod"?
If yes: we could pattern match for such "expanded" versions of Div/Mod, but it would be quite complex: you would have to parse through patterns like displayed up here. Do you think that is a good idea? I already mentioned the idea up here, but did not think it was desirable due to the complexity.

Alternatively, as part of the expansion, new representation can be wrapped in CastII/CastLL with the narrower type of original Div/Mod node.

How does this "wrapping" help? After parsing, the CastII at the bottom of the "expanded" Div would just have the whole int range. How would the type of the CastII ever be improved, without pattern matching the "expanded" Div?

It seems the root cause is a divergence in functionality in GVN between different representations of Div/Mod nodes.

How hard would it be to align the implementation and improve GVN handling for other representations, so the same type is constructed irrespective of the IR shape?

For Div, we can have either the magic multiply/shift variant, or shifting for powers of 2. Each of these can have slightly different shapes depending on e.g., the sign of the divisor, the sign of the dividend, and the sign of the magic constant.
For DivL, we also use MulHi if supported and a different sequence of instructions otherwise.

For Mod, we either do what we do for Div, multiply again and subtract to get the remainder; or we just directly use And, or we do the Mersenne number optimization (related: https://bugs.openjdk.org/browse/JDK-8370135) which unrolls the same few operations multiple times.

Generally, there also isn't one guaranteed result node (like e.g., Sub) where we could place code that recognizes these patterns and provides better results, so I don't think this is feasible (for Div it might be doable, at least I only found that off-by-one overapproximation that could be dealt with in Sub).

Alternatively, as part of the expansion, new representation can be wrapped in CastII/CastLL with the narrower type of original Div/Mod node.

I thought about that a bit as well and I think it has the same downside as the current approach: As soon as we don't use the Div/Mod node anymore, making the inputs more precise doesn't help anymore. We still have the Cast node, but that node doesn't know how to recalculate/improve its own type.
(Additionally, but less of a problem, a Cast node would require optimizations checking their inputs for Div/Mod nodes to uncast).

Basically, this comes back to what e-graphs do better: remember multiple alternative constructs for the same semantic operation. Without considering whether that's realistic, if a Cast node would keep the original operation alive somehow (but that operation isn't further optimized itself, I guess), then the Cast node could recalculate its type depending on multiple variants and choose the more specific result even at later stages of optimization.

That said, I'm not in the position if using Cast nodes is more idiomatic, and I'm open to rework the PR to use Cast nodes if you want.

@SirYwell We were probably typing at the same time ;)

@SirYwell We were probably typing at the same time ;)

Indeed :)

I still updated the comments now as they are useful independently of the ongoing discussion. Feel free to give your opinion on that :)

I also noticed that x % 2 == 0 isn't optimized to (x & 1) == 0, which is a bit surprising. I found https://bugs.openjdk.org/browse/JDK-8210152 but that seems to be different, so maybe there is a regression somewhere if that worked before?

Do you consider the "expanded" versions of Div/Mod as a "different representation of Div/Mod"?

Yes, exactly.

we could pattern match for such "expanded" versions of Div/Mod, but it would be quite complex: you would have to parse through patterns like displayed #27886 (comment). Do you think that is a good idea?

Sure, It may be way above the complexity budget we are willing to spend on it. The expansion code I see for Div/Mod nodes doesn't look too complicated, but matching the pattern may require more effort. The positive thing is it'll optimize the pattern irrespective of the origin (either expanded Div/Mod or explicitly optimized in the code by the user). So, the question is how much complexity it requires vs scenarios it covers.

How does this "wrapping" help? After parsing, the CastII at the bottom of the "expanded" Div would just have the whole int range. How would the type of the CastII ever be improved, without pattern matching the "expanded" Div?

It's not fully clear to me what is the scope of problematic scenarios. If it's only about Ideal() expanding the node before Value() has a chance to run, then wrapping the result of expansion in CastII/CastLL node and attach Value() as it's type should be enough (when produced type is narrower than Type::INT).

If we want to to keep expanded shape while being able to compute its type as if it were the original node, then a new flavor of Cast node may help. The one which keeps the node type and its inputs and can run Value() as if it were the original node.

@iwanowww I see, so we could implement something like a CastII with multiple inputs, which we know must all be identical at runtime. The first input is the one we will in the end pick. But during Value, we take the intersection of all input ranges. So if another (not the first input) has a narrower type, we can use that type. I suppose that would be feasible. Do you have a good name for such a node?

What I don't know: how does that interact with other IGVN optimizations, especially those that want to pattern match specific nodes? Inserting such special cast nodes could interrupt Ideal optimizations, current pattern matching would not know how to deal with it. Probably it is not a big issue, but I'm not sure.